From de9ee3c40848e978c65d87a7b4a3d04fb81a2d73 Mon Sep 17 00:00:00 2001 From: Elim Tsiagbey Date: Tue, 17 Dec 2024 11:22:55 -0500 Subject: [PATCH] feat: enhance service account key rotation with validation callback - add validation callback will enable the host service to validate the newly created key before the deleting the old key - wrapped both the call to validate permissions and validate callback in a retry because sometimes it takes some time for the newly created key to be active on GCP services. This throws an error and prevents host catalog creation --- internal/credential/config.go | 1 + internal/credential/rotate.go | 69 ++++++++++++++++++++++-------- internal/credential/rotate_test.go | 9 +++- internal/credential/state.go | 8 +++- plugin/service/host/plugin.go | 24 ++++++++++- plugin/service/host/state.go | 28 ++++++++++++ plugin/service/host/state_test.go | 43 +++++++++++++++++++ 7 files changed, 160 insertions(+), 22 deletions(-) diff --git a/internal/credential/config.go b/internal/credential/config.go index 4dbc11e..07dc8aa 100644 --- a/internal/credential/config.go +++ b/internal/credential/config.go @@ -98,6 +98,7 @@ func (c *Config) toCredentials() *credentials { func (c *Config) clone() *Config { return &Config{ ProjectId: c.ProjectId, + Zone: c.Zone, PrivateKey: c.PrivateKey, PrivateKeyId: c.PrivateKeyId, ClientEmail: c.ClientEmail, diff --git a/internal/credential/rotate.go b/internal/credential/rotate.go index 8fe8c38..29a22d5 100644 --- a/internal/credential/rotate.go +++ b/internal/credential/rotate.go @@ -6,8 +6,10 @@ package credential import ( "context" "encoding/json" + "errors" "fmt" "slices" + "time" admin "cloud.google.com/go/iam/admin/apiv1" "cloud.google.com/go/iam/admin/apiv1/adminpb" @@ -19,9 +21,20 @@ import ( ) const ( - ComputeInstancesListPermission = "compute.instances.list" + // ComputeInstancesListPermission is the IAM permission required + // to list compute instances. + ComputeInstancesListPermission = "compute.instances.list" + // IAMServiceAccountKeysCreatePermission is the IAM permission + // required to create a service account key. + // This permission is required to rotate service account keys. IAMServiceAccountKeysCreatePermission = "iam.serviceAccountKeys.create" + // IAMServiceAccountKeysDeletePermission is the IAM permission + // required to delete a service account key. + // This permission is required to rotate service account keys. IAMServiceAccountKeysDeletePermission = "iam.serviceAccountKeys.delete" + // validateServiceAccountKeyTimeout is the timeout for validating + // a service account key. + validateServiceAccountKeyTimeout = 5 * time.Second ) // ServiceAccountPrivateKey represents a decoded PrivateKeyData @@ -40,6 +53,8 @@ type ServiceAccountPrivateKey struct { ClientX509CertURL string `json:"client_x509_cert_url"` } +type ValidateCredsCallback func(*Config, ...option.ClientOption) error + // 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. @@ -47,7 +62,12 @@ type ServiceAccountPrivateKey struct { // If deletion of the old private key is successful, the new private key and // private key id are written into the credentials config and nil is returned. // On any error, the old credentials are not overwritten. -func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []string, opts ...option.ClientOption) error { +func (c *Config) RotateServiceAccountKey( + ctx context.Context, + permissions []string, + validateCredsCallback ValidateCredsCallback, + opts ...option.ClientOption, +) error { if c.PrivateKey == "" { return status.Error(codes.InvalidArgument, "cannot rotate credentials when private key is not set") } @@ -104,10 +124,10 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri clientOptions = []option.ClientOption{option.WithTokenSource(newCreds.TokenSource)} clientOptions = append(clientOptions, opts...) - // Validate that the new credentials have the necessary permissions. - _, err = newConfig.ValidateIamPermissions(ctx, permissions, clientOptions...) + // Validate the new service account key + err = newConfig.ValidateServiceAccountKey(ctx, permissions, validateCredsCallback, clientOptions...) if err != nil { - return status.Errorf(codes.PermissionDenied, "error testing IAM permissions with rotated service account key: %v", err) + return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v", err) } iamClient, err = admin.NewIamClient(ctx, clientOptions...) @@ -128,6 +148,32 @@ func (c *Config) RotateServiceAccountKey(ctx context.Context, permissions []stri return nil } +// ValidateServiceAccountKey validates the service account key by checking the IAM permissions +// and calling the validation callback. +// The function will retry validation until the timeout is reached. +func (c *Config) ValidateServiceAccountKey( + ctx context.Context, + permissions []string, + validateCredsCallback ValidateCredsCallback, + opts ...option.ClientOption) error { + var validatePermissionsErr error + var validationCallbackErr error + + for start := time.Now(); time.Since(start) < validateServiceAccountKeyTimeout; time.Sleep(200 * time.Millisecond) { + _, validatePermissionsErr = c.ValidateIamPermissions(ctx, permissions, opts...) + + if validateCredsCallback != nil { + validationCallbackErr = validateCredsCallback(c, opts...) + } + + if validatePermissionsErr == nil && validationCallbackErr == nil { + return nil + } + } + + return errors.Join(validatePermissionsErr, validationCallbackErr) +} + func (c *Config) DeletePrivateKey(ctx context.Context, opts ...option.ClientOption) error { if c.PrivateKey == "" { return status.Error(codes.InvalidArgument, "cannot delete credentials when private key is not set") @@ -172,18 +218,7 @@ func (c *Config) ValidateIamPermissions(ctx context.Context, permissions []strin return nil, status.Error(codes.InvalidArgument, "permissions are required") } - creds, err := c.GenerateCredentials(ctx) - if err != nil { - return nil, status.Errorf(codes.Unauthenticated, "error generating credentials: %v", err) - } - if creds.TokenSource == nil { - return nil, status.Error(codes.Unauthenticated, "error generating credentials: token source is nil") - } - - clientOptions := []option.ClientOption{option.WithTokenSource(creds.TokenSource)} - clientOptions = append(clientOptions, opts...) - - rmClient, err := resourcemanager.NewProjectsClient(ctx, clientOptions...) + rmClient, err := resourcemanager.NewProjectsClient(ctx, opts...) if err != nil { return nil, status.Errorf(codes.Internal, "failed to create Resource Manager client: %v", err) } diff --git a/internal/credential/rotate_test.go b/internal/credential/rotate_test.go index a13c3d1..9018ffd 100644 --- a/internal/credential/rotate_test.go +++ b/internal/credential/rotate_test.go @@ -128,8 +128,14 @@ func TestRotateServiceAccountKey(t *testing.T) { IAMServiceAccountKeysCreatePermission, IAMServiceAccountKeysDeletePermission, } + validateCredsCallbackCalled := false - err = tt.config.RotateServiceAccountKey(ctx, permissions, testOptions...) + validateCredsCallBack := func(c *Config, opts ...googleOption.ClientOption) error { + validateCredsCallbackCalled = true + return nil + } + + err = tt.config.RotateServiceAccountKey(ctx, permissions, validateCredsCallBack, testOptions...) if tt.expectedError != nil { require.ErrorContains(t, err, tt.expectedError.Error()) require.Equal(t, initialClientEmail, tt.config.ClientEmail) @@ -143,6 +149,7 @@ func TestRotateServiceAccountKey(t *testing.T) { require.NotEmpty(t, tt.config.PrivateKeyId) require.NotEqual(t, initialPrivateKey, tt.config.PrivateKey) require.NotEqual(t, initialPrivateKeyId, tt.config.PrivateKeyId) + require.True(t, validateCredsCallbackCalled) }) } } diff --git a/internal/credential/state.go b/internal/credential/state.go index 013c3ee..d9d27d5 100644 --- a/internal/credential/state.go +++ b/internal/credential/state.go @@ -38,7 +38,11 @@ func NewPersistedState(opt ...Option) (*PersistedState, error) { } // 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 { +func (s *PersistedState) RotateCreds( + ctx context.Context, + permissions []string, + validateCredsCallback ValidateCredsCallback, + opts ...option.ClientOption) error { if s.CredentialsConfig == nil { return status.Error(codes.InvalidArgument, "missing credentials config") } @@ -46,7 +50,7 @@ func (s *PersistedState) RotateCreds(ctx context.Context, permissions []string, return status.Error(codes.InvalidArgument, "cannot rotate non-rotatable credentials") } - err := s.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, opts...) + err := s.CredentialsConfig.RotateServiceAccountKey(ctx, permissions, validateCredsCallback, opts...) if err != nil { return status.Errorf(codes.Internal, "failed to rotate credentials: %v", err) } diff --git a/plugin/service/host/plugin.go b/plugin/service/host/plugin.go index 9dbe72d..5e8fbd0 100644 --- a/plugin/service/host/plugin.go +++ b/plugin/service/host/plugin.go @@ -88,7 +88,17 @@ func (p *HostPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalo } if credState.CredentialsConfig.IsRotatable() && !catalogAttributes.DisableCredentialRotation { - if err := credState.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil { + state, err := newGCPCatalogPersistedState( + append([]gcpCatalogPersistedStateOption{ + withCredentials(credState), + }, p.testCatalogStateOpts...)..., + ) + if err != nil { + return nil, status.Errorf(codes.Internal, "error setting up persisted state: %s", err) + } + validateCredsCallback := state.ValidateCredsCallbackFn(ctx, p.testGCPClientOpts...) + + if err := credState.RotateCreds(ctx, permissions, validateCredsCallback, p.testGCPClientOpts...); err != nil { return nil, err } } @@ -215,7 +225,17 @@ 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.RotateCreds(ctx, permissions, p.testGCPClientOpts...); err != nil { + state, err := newGCPCatalogPersistedState( + append([]gcpCatalogPersistedStateOption{ + withCredentials(credState), + }, p.testCatalogStateOpts...)..., + ) + if err != nil { + return nil, status.Errorf(codes.Internal, "error setting up persisted state: %s", err) + } + validateCredsCallback := state.ValidateCredsCallbackFn(ctx, p.testGCPClientOpts...) + + if err := credState.RotateCreds(ctx, permissions, validateCredsCallback, p.testGCPClientOpts...); err != nil { return nil, err } } diff --git a/plugin/service/host/state.go b/plugin/service/host/state.go index abf7448..6e4a1b5 100644 --- a/plugin/service/host/state.go +++ b/plugin/service/host/state.go @@ -13,6 +13,7 @@ import ( "github.com/googleapis/gax-go/v2" "github.com/hashicorp/boundary-plugin-gcp/internal/credential" pb "github.com/hashicorp/boundary/sdk/pbs/plugin" + "google.golang.org/api/iterator" "google.golang.org/api/option" "google.golang.org/protobuf/types/known/structpb" ) @@ -99,3 +100,30 @@ func (s *gcpCatalogPersistedState) InstancesClient( return client, nil } + +// validateCredsCallbackFn returns a ValidateCredsCallback function that +// validates the credentials by listing instances in the project and zone. +func (s *gcpCatalogPersistedState) ValidateCredsCallbackFn( + ctx context.Context, + opts ...option.ClientOption, +) credential.ValidateCredsCallback { + return func(config *credential.Config, opts ...option.ClientOption) error { + s.CredentialsConfig = config + instancesClient, err := s.InstancesClient(ctx, opts...) + if err != nil { + return fmt.Errorf("error creating instances client: %w", err) + } + + it := instancesClient.List(ctx, &computepb.ListInstancesRequest{ + Project: s.CredentialsConfig.ProjectId, + Zone: s.CredentialsConfig.Zone, + }) + + _, err = it.Next() + if err != nil && err != iterator.Done { + return fmt.Errorf("error listing instances: %w", err) + } + + return nil + } +} diff --git a/plugin/service/host/state_test.go b/plugin/service/host/state_test.go index b5a3ef2..ad739e7 100644 --- a/plugin/service/host/state_test.go +++ b/plugin/service/host/state_test.go @@ -5,9 +5,11 @@ package host import ( "context" + "errors" "testing" "time" + computepb "cloud.google.com/go/compute/apiv1/computepb" "github.com/hashicorp/boundary-plugin-gcp/internal/credential" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -87,3 +89,44 @@ func TestInstancesClient(t *testing.T) { assert.Contains(t, err.Error(), "error creating instances client") }) } +func TestValidateCredsCallbackFn(t *testing.T) { + ctx := context.Background() + cred := &credential.PersistedState{ + CredentialsConfig: &credential.Config{ + ProjectId: "test-project-id", + Zone: "test-zone", + }, + } + state := &gcpCatalogPersistedState{PersistedState: cred} + + t.Run("List instances error", func(t *testing.T) { + state.testInstancesAPIFunc = newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(nil), + testMockInstancesWithListInstancesError(errors.New("error listing instances")), + ) + + callback := state.ValidateCredsCallbackFn(ctx) + err := callback(&credential.Config{ + ProjectId: "test-project-id", + Zone: "test-zone", + }) + assert.Error(t, err) + assert.Contains(t, err.Error(), "error listing instances") + }) + + t.Run("Success", func(t *testing.T) { + state.testInstancesAPIFunc = newTestMockInstances(ctx, + nil, + testMockInstancesWithListInstancesOutput(&computepb.InstanceList{}), + testMockInstancesWithListInstancesError(nil), + ) + + callback := state.ValidateCredsCallbackFn(ctx) + err := callback(&credential.Config{ + ProjectId: "test-project-id", + Zone: "test-zone", + }) + assert.NoError(t, err) + }) +}