fix: Cred rotation (#20)

* fix: Cred rotation

- fix bug where the rotated service account key was not read. The response needs to be unmarshalled to pull out the values from th fields
- fix bug where the time stamp was not getting updated after rotating credentials
- fix bug where updates without setting a new secret fails

internal/credential/attributes.go

@@ -115,13 +115,16 @@ func GetCredentialsConfig(secrets *structpb.Struct, attrs *CredentialAttributes)
 	}
 
 	switch {
-	// dynamic credentials requires the target service account id, cl and private key
-	case attrs.TargetServiceAccountId != "" && (privateKey == "" || attrs.ClientEmail == ""):
+	// service account impersonation authentication requires the target service account id and client_email
+	case attrs.TargetServiceAccountId != "" && attrs.ClientEmail == "":
 		badFields[fmt.Sprintf("secrets.%s", ConstPrivateKey)] = "must not be empty when target service account id is set"
+	// service account impersonation authentication requires the target service account id and private key
+	case attrs.TargetServiceAccountId != "" && privateKey == "":
 		badFields[fmt.Sprintf("attributes.%s", ConstClientEmail)] = "must not be empty when target service account id is set"
-	// static credentials requires the private key and client email
+	// service account key credentials requires the private key and client email
 	case privateKey != "" && attrs.ClientEmail == "":
 		badFields[fmt.Sprintf("attributes.%s", ConstClientEmail)] = "must not be empty when private key is set"
+	// service account key credentials requires the client_email
 	case attrs.ClientEmail != "" && privateKey == "":
 		badFields[fmt.Sprintf("secrets.%s", ConstPrivateKey)] = "must not be empty when client email is set"
 	}

internal/credential/rotate.go

@@ -5,9 +5,9 @@ package credential
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"slices"
-	"strings"
 
 	admin "cloud.google.com/go/iam/admin/apiv1"
 	"cloud.google.com/go/iam/admin/apiv1/adminpb"
@@ -22,9 +22,24 @@ const (
 	ComputeInstancesListPermission        = "compute.instances.list"
 	IAMServiceAccountKeysCreatePermission = "iam.serviceAccountKeys.create"
 	IAMServiceAccountKeysDeletePermission = "iam.serviceAccountKeys.delete"
-	CreateServiceAccountKeyResourceName   = "projects/-/serviceAccounts"
 )
 
+// ServiceAccountPrivateKey represents a decoded PrivateKeyData
+// from a Service Account Key.
+// https://cloud.google.com/iam/docs/reference/rest/v1/projects.serviceAccounts.keys#ServiceAccountKey
+type ServiceAccountPrivateKey struct {
+	Type                    string `json:"type"`
+	ProjectID               string `json:"project_id"`
+	PrivateKeyID            string `json:"private_key_id"`
+	PrivateKey              string `json:"private_key"`
+	ClientEmail             string `json:"client_email"`
+	ClientID                string `json:"client_id"`
+	AuthURI                 string `json:"auth_uri"`
+	TokenURI                string `json:"token_uri"`
+	AuthProviderX509CertURL string `json:"auth_provider_x509_cert_url"`
+	ClientX509CertURL       string `json:"client_x509_cert_url"`
+}
+
 // RotateServiceAccountKey takes the private key from this credentials config
 // and first creates a new private key and private key id, then deletes the
 // old private key.
@@ -60,7 +75,7 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri
 	}
 
 	createServiceAccountKeyRes, err := iamClient.CreateServiceAccountKey(ctx, &adminpb.CreateServiceAccountKeyRequest{
-		Name:           fmt.Sprintf("%s/%s", CreateServiceAccountKeyResourceName, c.ClientEmail),
+		Name:           fmt.Sprintf("projects/%s/serviceAccounts/%s", c.ProjectId, c.ClientEmail),
 		PrivateKeyType: adminpb.ServiceAccountPrivateKeyType_TYPE_GOOGLE_CREDENTIALS_FILE,
 		KeyAlgorithm:   adminpb.ServiceAccountKeyAlgorithm_KEY_ALG_RSA_2048,
 	})
@@ -68,15 +83,16 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri
 		return status.Errorf(codes.Internal, "error creating service account key: %v", err)
 	}
 
-	privateKeyId, err := getPrivateKeyIdFromName(createServiceAccountKeyRes.Name)
+	var serviceAccountKey ServiceAccountPrivateKey
+	err = json.Unmarshal(createServiceAccountKeyRes.PrivateKeyData, &serviceAccountKey)
 	if err != nil {
-		return status.Errorf(codes.Internal, "error parsing private key ID: %v", err)
+		return status.Errorf(codes.Internal, "error unmarshalling service account key: %v", err)
 	}
 
 	// Clone the config, update the private key and private key ID with the new key.
 	newConfig := c.clone()
-	newConfig.PrivateKey = string(createServiceAccountKeyRes.PrivateKeyData)
-	newConfig.PrivateKeyId = privateKeyId
+	newConfig.PrivateKey = serviceAccountKey.PrivateKey
+	newConfig.PrivateKeyId = serviceAccountKey.PrivateKeyID
 
 	newCreds, err := newConfig.GenerateCredentials(ctx)
 	if err != nil {
@@ -100,7 +116,7 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri
 	}
 
 	err = iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
-		Name: fmt.Sprintf("%s/%s/keys/%s", CreateServiceAccountKeyResourceName, c.ClientEmail, c.PrivateKeyId),
+		Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId),
 	})
 	if err != nil {
 		return status.Errorf(codes.Internal, "error deleting service account key: %v", err)
@@ -140,7 +156,7 @@ func (c *Config) DeletePrivateKey(ctx context.Context, opts ...option.ClientOpti
 	}
 
 	err = iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{
-		Name: fmt.Sprintf("projects/-/serviceAccounts/%s/keys/%s", c.ClientEmail, c.PrivateKeyId),
+		Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId),
 	})
 	if err != nil {
 		return status.Errorf(codes.Unknown, "error deleting service account key: %v", err)
@@ -197,12 +213,3 @@ func (c *Config) ValidateIamPermissions(ctx context.Context, permissions []strin
 
 	return resp.Permissions, nil
 }
-
-// getPrivateKeyIdFromName extracts the private key ID from the name of a service account key.
-func getPrivateKeyIdFromName(input string) (string, error) {
-	lastSlashIndex := strings.LastIndex(input, "/")
-	if lastSlashIndex != -1 {
-		return input[lastSlashIndex+1:], nil
-	}
-	return "", fmt.Errorf("could not find private key ID in %s", input)
-}

internal/credential/state.go

@@ -37,6 +37,24 @@ func NewPersistedState(opt ...Option) (*PersistedState, error) {
 	return s, nil
 }
 
+// RotateCreds rotates the credentials for the GCP catalog and updates the last rotated time.
+func (s *PersistedState) RotateCreds(ctx context.Context, permissions []string, opts ...option.ClientOption) error {
+	if s.CredentialsConfig == nil {
+		return status.Error(codes.InvalidArgument, "missing credentials config")
+	}
+	if !s.CredentialsConfig.IsRotatable() {
+		return status.Error(codes.InvalidArgument, "cannot rotate non-rotatable credentials")
+	}
+
+	err := s.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, opts...)
+	if err != nil {
+		return status.Errorf(codes.Internal, "failed to rotate credentials: %v", err)
+	}
+	s.CredsLastRotatedTime = time.Now()
+
+	return nil
+}
+
 // ReplaceCreds replaces the private key in the state with a new key.
 // If the existing key was rotated at any point in time, it is
 // deleted first, otherwise it's left alone. This method returns a

internal/credential/testing.go

@@ -5,6 +5,7 @@ package credential
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net"
 
@@ -29,9 +30,17 @@ func NewTestIAMAdminServer(createServiceAccountKeyError error, deleteServiceAcco
 }
 
 func (f *testIAMAdminServer) CreateServiceAccountKey(ctx context.Context, req *adminpb.CreateServiceAccountKeyRequest) (*adminpb.ServiceAccountKey, error) {
+	serviceAccountKey := ServiceAccountPrivateKey{
+		PrivateKey:   "updated-private-key",
+		PrivateKeyID: "updated-private-key-id",
+	}
+	serviceAccountKeyData, err := json.Marshal(serviceAccountKey)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal service account key: %v", err)
+	}
 	resp := &adminpb.ServiceAccountKey{
 		Name:           "projects/-/serviceAccounts/1234/keys/updated-private-key-id",
-		PrivateKeyData: []byte("updated-private-key"),
+		PrivateKeyData: serviceAccountKeyData,
 	}
 	return resp, f.testCreateServiceAccountKeyError
 }

plugin/service/host/plugin.go

@@ -88,7 +88,7 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo
 	}
 
 	if credState.CredentialsConfig.IsRotatable() && !catalogAttributes.DisableCredentialRotation {
-		if err := credState.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, p.testGCPClientOpts...); err != nil {
+		if err := credState.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil {
 			return nil, err
 		}
 	}
@@ -152,12 +152,15 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo
 		return nil, status.Errorf(codes.InvalidArgument, "error getting persisted state from proto: %s", err)
 	}
 
-	updatedCredentials, err := credential.GetCredentialsConfig(newCatalog.GetSecrets(), newCatalogAttributes.CredentialAttributes)
-	if err != nil {
-		return nil, err
-	}
+	// We will be updating credentials when one of two changes occur:
+	// 1. The new catalog has a service account key, in which case we need to delete the old service account key credentials
+	// 2. The target service account ID has changed. This indicates a change of credential type to use service account impersonation
+	if newCatalog.GetSecrets() != nil || newCatalogAttributes.TargetServiceAccountId != oldCatalogAttributes.TargetServiceAccountId {
+		updatedCredentials, err := credential.GetCredentialsConfig(newCatalog.GetSecrets(), newCatalogAttributes.CredentialAttributes)
+		if err != nil {
+			return nil, err
+		}
 
-	if newCatalog.GetSecrets() != nil {
 		newCredState, err := credential.NewPersistedState([]credential.Option{
 			credential.WithCredentialsConfig(updatedCredentials),
 		}...,
@@ -186,11 +189,14 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo
 		}
 	}
 
-	if credState.CredentialsConfig.IsRotatable() {
-		if !updatedCredentials.IsRotatable() && !newCatalogAttributes.DisableCredentialRotation {
-			return nil, status.Error(codes.InvalidArgument, "cannot rotate credentials for non-rotatable credentials")
-		}
+	// This is a validate check to make sure that we aren't trying to
+	// rotate credentials that are not rotatable. For example,
+	// Application Default Credentials (ADC) are not rotatable.
+	if !credState.CredentialsConfig.IsRotatable() && !newCatalogAttributes.DisableCredentialRotation {
+		return nil, status.Error(codes.InvalidArgument, "cannot rotate credentials for non-rotatable credentials")
+	}
 
+	if credState.CredentialsConfig.IsRotatable() {
 		// This is a validate check to make sure that we aren't disabling
 		// rotation for credentials currently being managed by rotation.
 		// This is not allowed.
@@ -209,7 +215,7 @@ func (p *HostPlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalo
 		// If we're enabling rotation now but didn't before, or have
 		// freshly replaced credentials, we can rotate here.
 		if !newCatalogAttributes.DisableCredentialRotation && credState.CredsLastRotatedTime.IsZero() {
-			if err := credState.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, p.testGCPClientOpts...); err != nil {
+			if err := credState.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil {
 				return nil, err
 			}
 		}

plugin/service/host/plugin_test.go

@@ -1172,6 +1172,12 @@ func TestUpdateCatalog(t *testing.T) {
 							},
 						},
 					},
+					Secrets: &structpb.Struct{
+						Fields: map[string]*structpb.Value{
+							cred.ConstPrivateKeyId: structpb.NewStringValue(""),
+							cred.ConstPrivateKey:   structpb.NewStringValue(""),
+						},
+					},
 				},
 				Persisted: &pb.HostCatalogPersisted{
 					Secrets: &structpb.Struct{
@@ -1183,8 +1189,55 @@ func TestUpdateCatalog(t *testing.T) {
 					},
 				},
 			},
+			catalogOpts: []gcpCatalogPersistedStateOption{
+				withTestInstancesAPIFunc(newTestMockInstances(ctx,
+					nil,
+					testMockInstancesWithListInstancesOutput(&computepb.InstanceList{}),
+					testMockInstancesWithListInstancesError(nil),
+				)),
+			},
 			expectedErr: "cannot rotate credentials for non-rotatable credentials",
 		},
+		{
+			name: "update application default credentials",
+			req: &pb.OnUpdateCatalogRequest{
+				CurrentCatalog: &hostcatalogs.HostCatalog{
+					Attrs: &hostcatalogs.HostCatalog_Attributes{
+						Attributes: &structpb.Struct{
+							Fields: map[string]*structpb.Value{
+								cred.ConstProjectId:                 structpb.NewStringValue("test-project"),
+								cred.ConstClientEmail:               structpb.NewStringValue("test@example.com"),
+								cred.ConstZone:                      structpb.NewStringValue("us-central1-a"),
+								cred.ConstDisableCredentialRotation: structpb.NewBoolValue(true),
+							},
+						},
+					},
+				},
+				NewCatalog: &hostcatalogs.HostCatalog{
+					Attrs: &hostcatalogs.HostCatalog_Attributes{
+						Attributes: &structpb.Struct{
+							Fields: map[string]*structpb.Value{
+								cred.ConstProjectId:                 structpb.NewStringValue("test-project-1"),
+								cred.ConstZone:                      structpb.NewStringValue("us-central1"),
+								cred.ConstDisableCredentialRotation: structpb.NewBoolValue(true),
+							},
+						},
+					},
+				},
+			},
+			catalogOpts: []gcpCatalogPersistedStateOption{
+				withTestInstancesAPIFunc(newTestMockInstances(ctx,
+					nil,
+					testMockInstancesWithListInstancesOutput(&computepb.InstanceList{}),
+					testMockInstancesWithListInstancesError(nil),
+				)),
+			},
+			expectedRsp: &pb.OnUpdateCatalogResponse{
+				Persisted: &pb.HostCatalogPersisted{
+					Secrets: &structpb.Struct{},
+				},
+			},
+		},
 	}
 
 	for _, tc := range cases {