From baa2c7a2e742dc23659fa9e10f89d13795e422d6 Mon Sep 17 00:00:00 2001 From: Elim Tsiagbey Date: Wed, 20 Nov 2024 10:22:21 -0500 Subject: [PATCH] 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 | 9 +++-- internal/credential/rotate.go | 43 ++++++++++++++---------- internal/credential/state.go | 18 ++++++++++ internal/credential/testing.go | 11 ++++++- plugin/service/host/plugin.go | 28 +++++++++------- plugin/service/host/plugin_test.go | 53 ++++++++++++++++++++++++++++++ 6 files changed, 129 insertions(+), 33 deletions(-) diff --git a/internal/credential/attributes.go b/internal/credential/attributes.go index b344b52..528604e 100644 --- a/internal/credential/attributes.go +++ b/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" } diff --git a/internal/credential/rotate.go b/internal/credential/rotate.go index 6c4b31b..8fe8c38 100644 --- a/internal/credential/rotate.go +++ b/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) -} diff --git a/internal/credential/state.go b/internal/credential/state.go index 167392c..013c3ee 100644 --- a/internal/credential/state.go +++ b/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 diff --git a/internal/credential/testing.go b/internal/credential/testing.go index ba9ab6a..0ba2e39 100644 --- a/internal/credential/testing.go +++ b/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 } diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index c28521c..9dbe72d 100644 --- a/plugin/service/host/plugin.go +++ b/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 } } diff --git a/plugin/service/host/plugin_test.go b/plugin/service/host/plugin_test.go index 3954d6a..33ed630 100644 --- a/plugin/service/host/plugin_test.go +++ b/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 {