From bf376b5480e0e78631b9a9e460a9358a6ffdaddc Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Fri, 1 May 2020 14:56:51 -0400 Subject: [PATCH 1/4] bug: service account key should use config ttl --- plugin/path_role_set_test.go | 14 ++-- plugin/secrets_service_account_key.go | 10 ++- plugin/secrets_test.go | 92 +++++++++++++++++++++++---- 3 files changed, 97 insertions(+), 19 deletions(-) diff --git a/plugin/path_role_set_test.go b/plugin/path_role_set_test.go index a26dd31d..7d5aa133 100644 --- a/plugin/path_role_set_test.go +++ b/plugin/path_role_set_test.go @@ -24,7 +24,7 @@ func TestPathRoleSet_Basic(t *testing.T) { "roles/viewer": struct{}{}, } - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -53,6 +53,7 @@ func TestPathRoleSet_Basic(t *testing.T) { if respData == nil { t.Fatalf("expected role set to have been created") } + verifyReadData(t, respData, map[string]interface{}{ "secret_type": SecretTypeAccessToken, // default "project": td.Project, @@ -79,7 +80,7 @@ func TestPathRoleSet_UpdateKeyRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, initRoles.Union(updatedRoles)) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -179,7 +180,7 @@ func TestPathRoleSet_RotateKeyRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -239,7 +240,7 @@ func TestPathRoleSet_UpdateTokenRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, initRoles.Union(updatedRoles)) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -333,7 +334,7 @@ func TestPathRoleSet_RotateTokenRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -657,7 +658,7 @@ type testData struct { IamAdmin *iam.Service } -func setupTest(t *testing.T) *testData { +func setupTest(t *testing.T, ttl string) *testData { proj := util.GetTestProject(t) credsJson, creds := util.GetTestCredentials(t) httpC, err := gcputil.GetHttpClient(creds, iam.CloudPlatformScope) @@ -673,6 +674,7 @@ func setupTest(t *testing.T) *testData { b, reqStorage := getTestBackend(t) testConfigUpdate(t, b, reqStorage, map[string]interface{}{ "credentials": credsJson, + "ttl": ttl, }) return &testData{ diff --git a/plugin/secrets_service_account_key.go b/plugin/secrets_service_account_key.go index 4279c2fd..e25c2971 100644 --- a/plugin/secrets_service_account_key.go +++ b/plugin/secrets_service_account_key.go @@ -58,7 +58,7 @@ func pathSecretServiceAccountKey(b *backend) *framework.Path { Description: fmt.Sprintf(`Private key type for service account key - defaults to %s"`, privateKeyTypeJson), Default: privateKeyTypeJson, }, - "ttl": &framework.FieldSchema{ + "ttl": { Type: framework.TypeDurationSecond, Description: "Lifetime of the service account key", }, @@ -79,6 +79,14 @@ func (b *backend) pathServiceAccountKey(ctx context.Context, req *logical.Reques keyAlg := d.Get("key_algorithm").(string) ttl := d.Get("ttl").(int) + if ttl == 0 { + conf, err := getConfig(ctx, req.Storage) + if err != nil { + return logical.ErrorResponse(fmt.Sprintf("error reading config: %s", err)), nil + } + ttl = int(conf.TTL.Seconds()) + } + rs, err := getRoleSet(rsName, ctx, req.Storage) if err != nil { return nil, err diff --git a/plugin/secrets_test.go b/plugin/secrets_test.go index 52f94839..53331e5c 100644 --- a/plugin/secrets_test.go +++ b/plugin/secrets_test.go @@ -33,7 +33,7 @@ func TestSecrets_GenerateAccessToken(t *testing.T) { secretType := SecretTypeAccessToken rsName := "test-gentoken" - td := setupTest(t) + td := setupTest(t, "0s") defer cleanup(t, td, rsName, testRoles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -69,11 +69,11 @@ func TestSecrets_GenerateAccessToken(t *testing.T) { verifyProjectBindingsRemoved(t, td, sa.Email, testRoles) } -func TestSecrets_GenerateKey(t *testing.T) { +func TestSecrets_GenerateKeyConfigTTL(t *testing.T) { secretType := SecretTypeKey rsName := "test-genkey" - td := setupTest(t) + td := setupTest(t, "1h") defer cleanup(t, td, rsName, testRoles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -95,13 +95,78 @@ func TestSecrets_GenerateKey(t *testing.T) { // expect error for trying to read token from key roleset testGetTokenFail(t, td, rsName) - creds, secret := testGetKey(t, td, rsName) + creds, resp := testGetKey(t, td, rsName, "") + if int(resp.Secret.LeaseTotal().Hours()) != 1 { + t.Fatalf("expected lease duration %d, got %d", 1, int(resp.Secret.LeaseTotal().Hours())) + } + + // Confirm calls with key work + keyHttpC := oauth2.NewClient(context.Background(), creds.TokenSource) + checkSecretPermissions(t, td, keyHttpC) + + keyName := resp.Secret.InternalData["key_name"].(string) + if keyName == "" { + t.Fatalf("expected internal data to include key name") + } + + _, err = td.IamAdmin.Projects.ServiceAccounts.Keys.Get(keyName).Do() + if err != nil { + t.Fatalf("could not get key from given internal 'key_name': %v", err) + } + + testRenewSecretKey(t, td, resp.Secret) + testRevokeSecretKey(t, td, resp.Secret) + + k, err := td.IamAdmin.Projects.ServiceAccounts.Keys.Get(keyName).Do() + + if k != nil { + t.Fatalf("expected error as revoked key was deleted, instead got key: %v", k) + } + if err == nil || !isGoogleAccountKeyNotFoundErr(err) { + t.Fatalf("expected 404 error from getting deleted key, instead got error: %v", err) + } + + // Cleanup: Delete role set + testRoleSetDelete(t, td, rsName, sa.Name) + verifyProjectBindingsRemoved(t, td, sa.Email, testRoles) +} + +func TestSecrets_GenerateKeyTTLOverride(t *testing.T) { + secretType := SecretTypeKey + rsName := "test-genkey" + + td := setupTest(t, "1h") + defer cleanup(t, td, rsName, testRoles) + + projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) + + // Create new role set + expectedBinds := ResourceBindings{projRes: testRoles} + bindsRaw, err := util.BindingsHCL(expectedBinds) + if err != nil { + t.Fatalf("unable to convert resource bindings to HCL string: %v", err) + } + testRoleSetCreate(t, td, rsName, + map[string]interface{}{ + "secret_type": secretType, + "project": td.Project, + "bindings": bindsRaw, + }) + sa := getRoleSetAccount(t, td, rsName) + + // expect error for trying to read token from key roleset + testGetTokenFail(t, td, rsName) + + creds, resp := testGetKey(t, td, rsName, "60s") + if int(resp.Secret.LeaseTotal().Seconds()) != 60 { + t.Fatalf("expected lease duration %d, got %d", 60, int(resp.Secret.LeaseTotal().Seconds())) + } // Confirm calls with key work keyHttpC := oauth2.NewClient(context.Background(), creds.TokenSource) checkSecretPermissions(t, td, keyHttpC) - keyName := secret.InternalData["key_name"].(string) + keyName := resp.Secret.InternalData["key_name"].(string) if keyName == "" { t.Fatalf("expected internal data to include key name") } @@ -111,8 +176,8 @@ func TestSecrets_GenerateKey(t *testing.T) { t.Fatalf("could not get key from given internal 'key_name': %v", err) } - testRenewSecretKey(t, td, secret) - testRevokeSecretKey(t, td, secret) + testRenewSecretKey(t, td, resp.Secret) + testRevokeSecretKey(t, td, resp.Secret) k, err := td.IamAdmin.Projects.ServiceAccounts.Keys.Get(keyName).Do() @@ -211,11 +276,17 @@ func testGetToken(t *testing.T, td *testData, rsName string) (token string) { return tokenRaw.(string) } -func testGetKey(t *testing.T, td *testData, rsName string) (*google.Credentials, *logical.Secret) { +func testGetKey(t *testing.T, td *testData, rsName, ttl string) (*google.Credentials, *logical.Response) { + data := map[string]interface{}{} + if ttl != "" { + data["ttl"] = ttl + } + resp, err := td.B.HandleRequest(context.Background(), &logical.Request{ Operation: logical.ReadOperation, Path: fmt.Sprintf("key/%s", rsName), Storage: td.S, + Data: data, }) if err != nil { @@ -227,12 +298,9 @@ func testGetKey(t *testing.T, td *testData, rsName string) (*google.Credentials, if resp == nil || resp.Secret == nil { t.Fatalf("expected response with secret, got response: %v", resp) } - if resp.Secret.ExpirationTime().Sub(resp.Secret.IssueTime) > defaultLeaseTTLHr*time.Hour { - t.Fatalf("unexpected lease duration is longer than backend default") - } creds := getGoogleCredentials(t, resp.Data) - return creds, resp.Secret + return creds, resp } func testRenewSecretKey(t *testing.T, td *testData, sec *logical.Secret) { From f12473b4f9c8a6398dbf0515b0b88a69452d3b03 Mon Sep 17 00:00:00 2001 From: catsby Date: Tue, 5 May 2020 16:17:53 -0500 Subject: [PATCH 2/4] Use configured TTL unless overridden by request --- plugin/path_role_set_test.go | 20 +++-- plugin/secrets_service_account_key.go | 12 +-- plugin/secrets_test.go | 107 ++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 22 deletions(-) diff --git a/plugin/path_role_set_test.go b/plugin/path_role_set_test.go index 7d5aa133..37361e93 100644 --- a/plugin/path_role_set_test.go +++ b/plugin/path_role_set_test.go @@ -24,7 +24,7 @@ func TestPathRoleSet_Basic(t *testing.T) { "roles/viewer": struct{}{}, } - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -80,7 +80,7 @@ func TestPathRoleSet_UpdateKeyRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, initRoles.Union(updatedRoles)) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -180,7 +180,7 @@ func TestPathRoleSet_RotateKeyRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -240,7 +240,7 @@ func TestPathRoleSet_UpdateTokenRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, initRoles.Union(updatedRoles)) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -334,7 +334,7 @@ func TestPathRoleSet_RotateTokenRoleSet(t *testing.T) { } // Initial test set up - backend, initial config, test resources in project - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, roles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -658,7 +658,7 @@ type testData struct { IamAdmin *iam.Service } -func setupTest(t *testing.T, ttl string) *testData { +func setupTest(t *testing.T, ttl, maxTTL string) *testData { proj := util.GetTestProject(t) credsJson, creds := util.GetTestCredentials(t) httpC, err := gcputil.GetHttpClient(creds, iam.CloudPlatformScope) @@ -672,10 +672,14 @@ func setupTest(t *testing.T, ttl string) *testData { } b, reqStorage := getTestBackend(t) - testConfigUpdate(t, b, reqStorage, map[string]interface{}{ + cfgData := map[string]interface{}{ "credentials": credsJson, "ttl": ttl, - }) + } + if maxTTL != "" { + cfgData["max_ttl"] = maxTTL + } + testConfigUpdate(t, b, reqStorage, cfgData) return &testData{ B: b, diff --git a/plugin/secrets_service_account_key.go b/plugin/secrets_service_account_key.go index e25c2971..41165252 100644 --- a/plugin/secrets_service_account_key.go +++ b/plugin/secrets_service_account_key.go @@ -79,14 +79,6 @@ func (b *backend) pathServiceAccountKey(ctx context.Context, req *logical.Reques keyAlg := d.Get("key_algorithm").(string) ttl := d.Get("ttl").(int) - if ttl == 0 { - conf, err := getConfig(ctx, req.Storage) - if err != nil { - return logical.ErrorResponse(fmt.Sprintf("error reading config: %s", err)), nil - } - ttl = int(conf.TTL.Seconds()) - } - rs, err := getRoleSet(rsName, ctx, req.Storage) if err != nil { return nil, err @@ -223,6 +215,10 @@ func (b *backend) getSecretKey(ctx context.Context, s logical.Storage, rs *RoleS resp := b.Secret(SecretTypeKey).Response(secretD, internalD) resp.Secret.Renewable = true + resp.Secret.MaxTTL = cfg.MaxTTL + resp.Secret.TTL = cfg.TTL + + // If the request came with a TTL value, overwrite the config default if ttl > 0 { resp.Secret.TTL = time.Duration(ttl) * time.Second } diff --git a/plugin/secrets_test.go b/plugin/secrets_test.go index 53331e5c..65a54370 100644 --- a/plugin/secrets_test.go +++ b/plugin/secrets_test.go @@ -33,7 +33,7 @@ func TestSecrets_GenerateAccessToken(t *testing.T) { secretType := SecretTypeAccessToken rsName := "test-gentoken" - td := setupTest(t, "0s") + td := setupTest(t, "0s", "2h") defer cleanup(t, td, rsName, testRoles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -73,7 +73,7 @@ func TestSecrets_GenerateKeyConfigTTL(t *testing.T) { secretType := SecretTypeKey rsName := "test-genkey" - td := setupTest(t, "1h") + td := setupTest(t, "1h", "2h") defer cleanup(t, td, rsName, testRoles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -95,7 +95,7 @@ func TestSecrets_GenerateKeyConfigTTL(t *testing.T) { // expect error for trying to read token from key roleset testGetTokenFail(t, td, rsName) - creds, resp := testGetKey(t, td, rsName, "") + creds, resp := testGetKey(t, td, rsName) if int(resp.Secret.LeaseTotal().Hours()) != 1 { t.Fatalf("expected lease duration %d, got %d", 1, int(resp.Secret.LeaseTotal().Hours())) } @@ -135,7 +135,7 @@ func TestSecrets_GenerateKeyTTLOverride(t *testing.T) { secretType := SecretTypeKey rsName := "test-genkey" - td := setupTest(t, "1h") + td := setupTest(t, "1h", "2h") defer cleanup(t, td, rsName, testRoles) projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) @@ -157,7 +157,8 @@ func TestSecrets_GenerateKeyTTLOverride(t *testing.T) { // expect error for trying to read token from key roleset testGetTokenFail(t, td, rsName) - creds, resp := testGetKey(t, td, rsName, "60s") + // call the POST endpoint of /gcp/key/:roleset with updated TTL + creds, resp := testPostKey(t, td, rsName, "60s") if int(resp.Secret.LeaseTotal().Seconds()) != 60 { t.Fatalf("expected lease duration %d, got %d", 60, int(resp.Secret.LeaseTotal().Seconds())) } @@ -193,6 +194,75 @@ func TestSecrets_GenerateKeyTTLOverride(t *testing.T) { verifyProjectBindingsRemoved(t, td, sa.Email, testRoles) } +// TestSecrets_GenerateKeyMaxTTLCheck verifies the MaxTTL is set for the +// configured backend +func TestSecrets_GenerateKeyMaxTTLCheck(t *testing.T) { + secretType := SecretTypeKey + rsName := "test-genkey" + + td := setupTest(t, "1h", "2h") + defer cleanup(t, td, rsName, testRoles) + + projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project) + + // Create new role set + expectedBinds := ResourceBindings{projRes: testRoles} + bindsRaw, err := util.BindingsHCL(expectedBinds) + if err != nil { + t.Fatalf("unable to convert resource bindings to HCL string: %v", err) + } + testRoleSetCreate(t, td, rsName, + map[string]interface{}{ + "secret_type": secretType, + "project": td.Project, + "bindings": bindsRaw, + }) + sa := getRoleSetAccount(t, td, rsName) + + // expect error for trying to read token from key roleset + testGetTokenFail(t, td, rsName) + + // call the POST endpoint of /gcp/key/:roleset with updated TTL + creds, resp := testPostKey(t, td, rsName, "60s") + if int(resp.Secret.LeaseTotal().Seconds()) != 60 { + t.Fatalf("expected lease duration %d, got %d", 60, int(resp.Secret.LeaseTotal().Seconds())) + } + + if int(resp.Secret.LeaseOptions.MaxTTL.Hours()) != 2 { + t.Fatalf("expected max lease %d, got %d", 2, int(resp.Secret.LeaseOptions.MaxTTL.Hours())) + } + + // Confirm calls with key work + keyHttpC := oauth2.NewClient(context.Background(), creds.TokenSource) + checkSecretPermissions(t, td, keyHttpC) + + keyName := resp.Secret.InternalData["key_name"].(string) + if keyName == "" { + t.Fatalf("expected internal data to include key name") + } + + _, err = td.IamAdmin.Projects.ServiceAccounts.Keys.Get(keyName).Do() + if err != nil { + t.Fatalf("could not get key from given internal 'key_name': %v", err) + } + + testRenewSecretKey(t, td, resp.Secret) + testRevokeSecretKey(t, td, resp.Secret) + + k, err := td.IamAdmin.Projects.ServiceAccounts.Keys.Get(keyName).Do() + + if k != nil { + t.Fatalf("expected error as revoked key was deleted, instead got key: %v", k) + } + if err == nil || !isGoogleAccountKeyNotFoundErr(err) { + t.Fatalf("expected 404 error from getting deleted key, instead got error: %v", err) + } + + // Cleanup: Delete role set + testRoleSetDelete(t, td, rsName, sa.Name) + verifyProjectBindingsRemoved(t, td, sa.Email, testRoles) +} + func getRoleSetAccount(t *testing.T, td *testData, rsName string) *iam.ServiceAccount { rs, err := getRoleSet(rsName, context.Background(), td.S) if err != nil { @@ -276,12 +346,37 @@ func testGetToken(t *testing.T, td *testData, rsName string) (token string) { return tokenRaw.(string) } -func testGetKey(t *testing.T, td *testData, rsName, ttl string) (*google.Credentials, *logical.Response) { +// testPostKey enables the POST call to /gcp/key/:roleset +func testPostKey(t *testing.T, td *testData, rsName, ttl string) (*google.Credentials, *logical.Response) { data := map[string]interface{}{} if ttl != "" { data["ttl"] = ttl } + resp, err := td.B.HandleRequest(context.Background(), &logical.Request{ + Operation: logical.UpdateOperation, + Path: fmt.Sprintf("key/%s", rsName), + Storage: td.S, + Data: data, + }) + + if err != nil { + t.Fatal(err) + } + if resp != nil && resp.IsError() { + t.Fatal(resp.Error()) + } + if resp == nil || resp.Secret == nil { + t.Fatalf("expected response with secret, got response: %v", resp) + } + + creds := getGoogleCredentials(t, resp.Data) + return creds, resp +} + +func testGetKey(t *testing.T, td *testData, rsName string) (*google.Credentials, *logical.Response) { + data := map[string]interface{}{} + resp, err := td.B.HandleRequest(context.Background(), &logical.Request{ Operation: logical.ReadOperation, Path: fmt.Sprintf("key/%s", rsName), From 6245da869f1a97e81cdebae3a07930efc4340a32 Mon Sep 17 00:00:00 2001 From: catsby Date: Tue, 5 May 2020 16:18:01 -0500 Subject: [PATCH 3/4] update makefile --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 6992166d..c17726bc 100644 --- a/Makefile +++ b/Makefile @@ -29,10 +29,10 @@ testcompile: fmtcheck generate done test: - @go test -short -parallel=40 ./... + @go test -short -parallel=40 ./... $(TESTARGS) test-acc: - @go test -parallel=40 $(TESTARGS) + @go test -parallel=40 ./... $(TESTARGS) # generate runs `go generate` to build the dynamically generated # source files. generate: From 7f941b2c44a58773c34954df258e87cd9aeaf82d Mon Sep 17 00:00:00 2001 From: catsby Date: Tue, 5 May 2020 16:22:44 -0500 Subject: [PATCH 4/4] code cleanup --- plugin/path_role_set_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/plugin/path_role_set_test.go b/plugin/path_role_set_test.go index 37361e93..25edf36a 100644 --- a/plugin/path_role_set_test.go +++ b/plugin/path_role_set_test.go @@ -672,14 +672,12 @@ func setupTest(t *testing.T, ttl, maxTTL string) *testData { } b, reqStorage := getTestBackend(t) - cfgData := map[string]interface{}{ + + testConfigUpdate(t, b, reqStorage, map[string]interface{}{ "credentials": credsJson, "ttl": ttl, - } - if maxTTL != "" { - cfgData["max_ttl"] = maxTTL - } - testConfigUpdate(t, b, reqStorage, cfgData) + "max_ttl": maxTTL, + }) return &testData{ B: b,