bug: service account key should use config ttl (#85)

* bug: service account key should use config ttl

* Use configured TTL unless overridden by request

* update makefile

* code cleanup

Co-authored-by: catsby <[email protected]>

Author: jasonodonnell

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:

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", "2h")
 	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", "2h")
 	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", "2h")
 	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", "2h")
 	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", "2h")
 	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, maxTTL string) *testData {
 	proj := util.GetTestProject(t)
 	credsJson, creds := util.GetTestCredentials(t)
 	httpC, err := gcputil.GetHttpClient(creds, iam.CloudPlatformScope)
@@ -671,8 +672,11 @@ func setupTest(t *testing.T) *testData {
 	}
 
 	b, reqStorage := getTestBackend(t)
+
 	testConfigUpdate(t, b, reqStorage, map[string]interface{}{
 		"credentials": credsJson,
+		"ttl":         ttl,
+		"max_ttl":     maxTTL,
 	})
 
 	return &testData{

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",
 			},
@@ -215,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
 	}

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", "2h")
 	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", "2h")
 	defer cleanup(t, td, rsName, testRoles)
 
 	projRes := fmt.Sprintf(testProjectResourceTemplate, td.Project)
@@ -95,13 +95,148 @@ 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", "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()))
+	}
+
+	// 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)
+}
+
+// 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 := 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 +246,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 +346,18 @@ 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) {
+// 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.ReadOperation,
+		Operation: logical.UpdateOperation,
 		Path:      fmt.Sprintf("key/%s", rsName),
 		Storage:   td.S,
+		Data:      data,
 	})
 
 	if err != nil {
@@ -227,12 +369,33 @@ 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
+}
+
+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),
+		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.Secret
+	return creds, resp
 }
 
 func testRenewSecretKey(t *testing.T, td *testData, sec *logical.Secret) {