From 80ce68602a7b91fd641fe874c97f65848f2730b8 Mon Sep 17 00:00:00 2001 From: adityawalvekar Date: Tue, 7 Apr 2026 15:22:21 +0530 Subject: [PATCH] Address review comments --- internal/credential/rotate.go | 23 ++++++++++++------ internal/credential/rotate_test.go | 38 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/internal/credential/rotate.go b/internal/credential/rotate.go index f3f6120..1f7b6ed 100644 --- a/internal/credential/rotate.go +++ b/internal/credential/rotate.go @@ -60,6 +60,8 @@ type ValidateCredsCallback func(*Config, ...option.ClientOption) error // // 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. +// If validation fails or deleting the old private key fails, the function +// attempts to delete the newly created key as a rollback operation. // On any error, the old credentials are not overwritten. func (c *Config) RotateServiceAccountKey( ctx context.Context, @@ -126,23 +128,30 @@ func (c *Config) RotateServiceAccountKey( // Validate the new service account key err = newConfig.ValidateServiceAccountKey(ctx, permissions, validateCredsCallback, clientOptions...) if err != nil { - return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v", err) + // Roll back the new key if validation fails. + rollbackErr := iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{ + Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId), + }) + if rollbackErr != nil { + return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v; error rolling back new rotated service account key: %v", err, rollbackErr) + } + + return status.Errorf(codes.PermissionDenied, "error validating rotated service account key: %v; successfully rolled back new rotated service account key", err) } - iamClient, err = admin.NewIamClient(ctx, clientOptions...) + newClient, err := admin.NewIamClient(ctx, clientOptions...) if err != nil { return status.Errorf(codes.Internal, "error creating IAM client with rotated service account key: %v", err) } - oldKeyName := fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId) - err = iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{ - Name: oldKeyName, + err = newClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{ + Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", c.ProjectId, c.ClientEmail, c.PrivateKeyId), }) // If deletion of the old key fails, attempt to delete the new key to avoid leaving a dangling key, then return an error. if err != nil { - newKeyName := fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId) + // delete the new key using old client since the new client may not have permissions that the old client has. rollbackErr := iamClient.DeleteServiceAccountKey(ctx, &adminpb.DeleteServiceAccountKeyRequest{ - Name: newKeyName, + Name: fmt.Sprintf("projects/%s/serviceAccounts/%s/keys/%s", newConfig.ProjectId, newConfig.ClientEmail, newConfig.PrivateKeyId), }) // If rollback also fails, return an error indicating both the failure to delete the old key and the failure to roll back the new key. if rollbackErr != nil { diff --git a/internal/credential/rotate_test.go b/internal/credential/rotate_test.go index fd08233..56c4450 100644 --- a/internal/credential/rotate_test.go +++ b/internal/credential/rotate_test.go @@ -72,9 +72,44 @@ func TestRotateServiceAccountKey(t *testing.T) { }, setup: func() { testIAMAdminServer.testCreateServiceAccountKeyError = errors.New("create key error") + testIAMAdminServer.testDeleteServiceAccountKeyError = nil + testIAMAdminServer.testDeleteServiceAccountKeyErrors = nil + testResourceServer.testIamPermissionsError = nil }, expectedError: errors.New("create key error"), }, + { + name: "ValidateServiceAccountKey fails and rollback succeeds", + config: &Config{ + PrivateKey: "some-private-key", + PrivateKeyId: "some-private-key-id", + ClientEmail: "client@example.com", + validateServiceAccountKeyTimeout: testValidateTimeout, + }, + setup: func() { + testIAMAdminServer.testCreateServiceAccountKeyError = nil + testIAMAdminServer.testDeleteServiceAccountKeyError = nil + testIAMAdminServer.testDeleteServiceAccountKeyErrors = []error{nil} + testResourceServer.testIamPermissionsError = errors.New("iam permissions error") + }, + expectedError: errors.New("successfully rolled back new rotated service account key"), + }, + { + name: "ValidateServiceAccountKey fails and rollback fails", + config: &Config{ + PrivateKey: "some-private-key", + PrivateKeyId: "some-private-key-id", + ClientEmail: "client@example.com", + validateServiceAccountKeyTimeout: testValidateTimeout, + }, + setup: func() { + testIAMAdminServer.testCreateServiceAccountKeyError = nil + testIAMAdminServer.testDeleteServiceAccountKeyError = nil + testIAMAdminServer.testDeleteServiceAccountKeyErrors = []error{errors.New("rollback key error")} + testResourceServer.testIamPermissionsError = errors.New("iam permissions error") + }, + expectedError: errors.New("error rolling back new rotated service account key"), + }, { name: "DeleteServiceAccountKey old key fails and rollback fails", config: &Config{ @@ -90,6 +125,7 @@ func TestRotateServiceAccountKey(t *testing.T) { errors.New("delete key error"), errors.New("rollback key error"), } + testResourceServer.testIamPermissionsError = nil }, expectedError: errors.New("error rolling back new rotated service account key"), }, @@ -108,6 +144,7 @@ func TestRotateServiceAccountKey(t *testing.T) { errors.New("delete key error"), nil, } + testResourceServer.testIamPermissionsError = nil }, expectedError: errors.New("successfully rolled back new rotated service account key"), }, @@ -123,6 +160,7 @@ func TestRotateServiceAccountKey(t *testing.T) { testIAMAdminServer.testCreateServiceAccountKeyError = nil testIAMAdminServer.testDeleteServiceAccountKeyError = nil testIAMAdminServer.testDeleteServiceAccountKeyErrors = nil + testResourceServer.testIamPermissionsError = nil }, expectedError: nil, },