Skip to content

Commit

Permalink
Refactored GCP client operations to log user messages and optimized r…
Browse files Browse the repository at this point in the history 
…esource updation

Ensured all operations modifying GCP cloud resources will log messages
to the user.

The 'ocm gcp update wif-config' command was unnecessarily updating the
oidc data of the workload identity pool, even when there were only
formatting differences. By improving the evaluation method for the jwks
configuration, the oidc configuration will now only be updated if there
is a meaningful difference.

Service Account access policies were being during every run of the
update command. By checking whether the policies are already in place,
updates to the policy will only occur if necassary.
  • Loading branch information
@renan-campos
renan-campos committed Jan 8, 2025
1 parent d4deb29 commit 4e0c787
Show file tree
Hide file tree
Showing 9 changed files with 341 additions and 113 deletions.
1 change: 0 additions & 1 deletion cmd/ocm/gcp/create-wif-config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@ func createWorkloadIdentityConfigurationCmd(cmd *cobra.Command, argv []string) e
return errors.Wrapf(err, "failed to initiate GCP client")
}

log.Println("Creating workload identity federation configuration...")
wifConfig, err := createWorkloadIdentityConfiguration(
ctx,
gcpClient,
Expand Down
175 changes: 137 additions & 38 deletions cmd/ocm/gcp/gcp-client-shim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ const (
retryDelayMs = 500
)

const (
impersonatorRole = "roles/iam.serviceAccountTokenCreator"
workloadIdentityUserRole = "roles/iam.workloadIdentityUser"
)

// All operations that modify cloud resources should be logged to the user.
// For this reason, all methods of this interface take a logger as a parameter.
type GcpClientWifConfigShim interface {
CreateServiceAccounts(ctx context.Context, log *log.Logger) error
CreateWorkloadIdentityPool(ctx context.Context, log *log.Logger) error
Expand Down Expand Up @@ -165,7 +172,7 @@ func (c *shim) CreateWorkloadIdentityProvider(
resp.DisplayName != providerId ||
resp.State != state ||
resp.Oidc.IssuerUri != issuerUrl ||
resp.Oidc.JwksJson != jwks ||
!utils.JwksEqual(resp.Oidc.JwksJson, jwks) ||
!reflect.DeepEqual(resp.AttributeMapping, attributeMap) ||
!reflect.DeepEqual(resp.Oidc.AllowedAudiences, audiences) {
needsUpdate = true
Expand Down Expand Up @@ -193,7 +200,7 @@ func (c *shim) CreateWorkloadIdentityProvider(
providerId, poolId,
)
}
log.Printf("Workload identity pool '%s' identity provider '%s' updated", poolId, providerId)
log.Printf("Workload identity pool '%s' identity provider '%s' was updated", poolId, providerId)
}
return nil
}
Expand All @@ -215,15 +222,15 @@ func (c *shim) CreateServiceAccounts(
); err != nil {
return err
}
log.Printf("IAM service account %s enabled", serviceAccount.ServiceAccountId())
log.Printf("IAM service account '%s' has been enabled", serviceAccount.ServiceAccountId())
}
if err := c.createOrUpdateRoles(ctx, log, serviceAccount.Roles()); err != nil {
return err
}
if err := c.bindRolesToServiceAccount(ctx, serviceAccount); err != nil {
if err := c.bindRolesToServiceAccount(ctx, log, serviceAccount); err != nil {
return err
}
if err := c.grantAccessToServiceAccount(ctx, serviceAccount); err != nil {
if err := c.grantAccessToServiceAccount(ctx, log, serviceAccount); err != nil {
return err
}
}
Expand All @@ -238,7 +245,7 @@ func (c *shim) GrantSupportAccess(
if err := c.createOrUpdateRoles(ctx, log, support.Roles()); err != nil {
return err
}
if err := c.bindRolesToGroup(ctx, support.Principal(), support.Roles()); err != nil {
if err := c.bindRolesToGroup(ctx, log, support.Principal(), support.Roles()); err != nil {
return err
}
return nil
Expand Down Expand Up @@ -282,7 +289,7 @@ func (c *shim) createServiceAccount(
if err != nil {
return nil, errors.Wrap(err, "Failed to create IAM service account")
}
log.Printf("IAM service account %s created", serviceAccountId)
log.Printf("IAM service account '%s' has been created", serviceAccountId)
return sa, nil
}

Expand Down Expand Up @@ -310,9 +317,9 @@ func (c *shim) createOrUpdateRoles(
c.wifConfig.Gcp().ProjectId(),
)
if err != nil {
return errors.Wrap(err, fmt.Sprintf("Failed to create %s", roleID))
return errors.Wrap(err, fmt.Sprintf("Failed to create role '%s'", roleID))
}
log.Printf("Role %q created", roleID)
log.Printf("Role '%s' has been created", roleID)
continue
} else {
return errors.Wrap(err, "Failed to check if role exists")
Expand All @@ -323,20 +330,20 @@ func (c *shim) createOrUpdateRoles(
if existingRole.Deleted {
_, err = c.undeleteRole(ctx, c.fmtRoleResourceId(role))
if err != nil {
return errors.Wrap(err, fmt.Sprintf("Failed to undelete custom role %q", roleID))
return errors.Wrap(err, fmt.Sprintf("Failed to undelete custom role '%s'", roleID))
}
existingRole.Deleted = false
log.Printf("Role %q undeleted", roleID)
log.Printf("Role '%s' has been undeleted", roleID)
}

// If role was disabled, enable role
if existingRole.Stage == adminpb.Role_DISABLED {
existingRole.Stage = adminpb.Role_GA
_, err := c.updateRole(ctx, existingRole, c.fmtRoleResourceId(role))
if err != nil {
return errors.Wrap(err, fmt.Sprintf("Failed to update %s", roleID))
return errors.Wrap(err, fmt.Sprintf("Failed to enable role '%s'", roleID))
}
log.Printf("Role %q enabled", roleID)
log.Printf("Role '%s' has been enabled", roleID)
}

if addedPermissions, needsUpdate := c.missingPermissions(permissions, existingRole.IncludedPermissions); needsUpdate {
Expand All @@ -346,9 +353,9 @@ func (c *shim) createOrUpdateRoles(

_, err := c.updateRole(ctx, existingRole, c.fmtRoleResourceId(role))
if err != nil {
return errors.Wrap(err, fmt.Sprintf("Failed to update %s", roleID))
return errors.Wrap(err, fmt.Sprintf("Failed to update role '%s'", roleID))
}
log.Printf("Role %q updated", roleID)
log.Printf("Role '%s' has been updated", roleID)
}
}
return nil
Expand Down Expand Up @@ -379,6 +386,7 @@ func (c *shim) missingPermissions(

func (c *shim) bindRolesToServiceAccount(
ctx context.Context,
log *log.Logger,
serviceAccount *cmv1.WifServiceAccount,
) error {
serviceAccountId := serviceAccount.ServiceAccountId()
Expand All @@ -391,6 +399,7 @@ func (c *shim) bindRolesToServiceAccount(
return utils.DelayedRetry(func() error {
return c.bindRolesToPrincipal(
ctx,
log,
fmt.Sprintf("serviceAccount:%s@%s.iam.gserviceaccount.com", serviceAccountId, c.wifConfig.Gcp().ProjectId()),
roles,
)
Expand All @@ -399,26 +408,29 @@ func (c *shim) bindRolesToServiceAccount(

func (c *shim) bindRolesToGroup(
ctx context.Context,
log *log.Logger,
groupEmail string,
roles []*cmv1.WifRole,
) error {
return c.bindRolesToPrincipal(
ctx,
log,
fmt.Sprintf("group:%s", groupEmail),
roles,
)
}

func (c *shim) bindRolesToPrincipal(
ctx context.Context,
log *log.Logger,
principal string,
roles []*cmv1.WifRole,
) error {
formattedRoles := make([]string, 0, len(roles))
for _, role := range roles {
formattedRoles = append(formattedRoles, c.fmtRoleResourceId(role))
}
err := c.ensurePolicyBindingsForProject(
modified, err := c.ensurePolicyBindingsForProject(
ctx,
formattedRoles,
principal,
Expand All @@ -427,34 +439,22 @@ func (c *shim) bindRolesToPrincipal(
if err != nil {
return errors.Errorf("Failed to bind roles to principal %s: %s", principal, err)
}
if modified {
log.Printf("Bound roles to principal '%s'", principal)
}
return nil
}

func (c *shim) grantAccessToServiceAccount(
ctx context.Context,
log *log.Logger,
serviceAccount *cmv1.WifServiceAccount,
) error {
switch serviceAccount.AccessMethod() {
case cmv1.WifAccessMethodImpersonate:
if err := c.gcpClient.AttachImpersonator(
ctx,
serviceAccount.ServiceAccountId(),
c.wifConfig.Gcp().ProjectId(),
c.wifConfig.Gcp().ImpersonatorEmail(),
); err != nil {
return errors.Wrapf(err, "Failed to attach impersonator to service account %s",
serviceAccount.ServiceAccountId())
}
return c.attachImpersonator(ctx, serviceAccount)
case cmv1.WifAccessMethodWif:
if err := c.gcpClient.AttachWorkloadIdentityPool(
ctx,
serviceAccount,
c.wifConfig.Gcp().WorkloadIdentityPool().PoolId(),
c.wifConfig.Gcp().ProjectId(),
); err != nil {
return errors.Wrapf(err, "Failed to attach workload identity pool to service account %s",
serviceAccount.ServiceAccountId())
}
return c.attachWorkloadIdentityPool(ctx, serviceAccount)
case cmv1.WifAccessMethodVm:
// Service accounts with the "vm" access method require no external access
return nil
Expand Down Expand Up @@ -487,6 +487,7 @@ func (c *shim) getRole(
}

// CreateRole creates a new role given permissions
// This method modifies cloud resources.
func (c *shim) createRole(
ctx context.Context,
permissions []string,
Expand All @@ -513,6 +514,7 @@ func (c *shim) createRole(

// UpdateRole updates an existing role given permissions.
// Custom roles should follow the format projects/{project}/roles/{role_id}.
// This method modifies cloud resources.
func (c *shim) updateRole(
ctx context.Context,
role *adminpb.Role,
Expand All @@ -529,6 +531,7 @@ func (c *shim) updateRole(
}

// UndeleteRole undeletes a previously deleted role that has not yet been pruned
// This method modifies cloud resources.
func (c *shim) undeleteRole(
ctx context.Context,
roleName string,
Expand All @@ -542,18 +545,19 @@ func (c *shim) undeleteRole(
// EnsurePolicyBindingsForProject ensures that given roles and member, appropriate binding is added to project.
// Roles should be in the format projects/{project}/roles/{role_id} for custom roles and roles/{role_id}
// for predefined roles.
// Return value indicates whether a modification occurred.
func (c *shim) ensurePolicyBindingsForProject(
ctx context.Context,
roles []string,
member string,
projectName string,
) error {
) (bool, error) {
needPolicyUpdate := false

policy, err := c.gcpClient.GetProjectIamPolicy(ctx, projectName, &cloudresourcemanager.GetIamPolicyRequest{})

if err != nil {
return fmt.Errorf("error fetching policy for project: %v", err)
return false, errors.Wrap(err, "Failed to fetch policy for project")
}

// Validate that each role exists, and add the policy binding as needed
Expand All @@ -569,13 +573,14 @@ func (c *shim) ensurePolicyBindingsForProject(
}

if needPolicyUpdate {
return c.setProjectIamPolicy(ctx, policy)
return true, c.setProjectIamPolicy(ctx, policy)
}

// If we made it this far there were no updates needed
return nil
return false, nil
}

// This method modifies cloud resources.
func (c *shim) setProjectIamPolicy(
ctx context.Context,
policy *cloudresourcemanager.Policy,
Expand Down Expand Up @@ -635,3 +640,97 @@ func (c *shim) createMemberRoleBindingForProject(
Role: roleName,
})
}

func (c *shim) attachImpersonator(
ctx context.Context,
serviceAccount *cmv1.WifServiceAccount,
) error {
policy, err := c.gcpClient.GetServiceAccountAccessPolicy(
ctx,
gcp.FmtSaResourceId(
serviceAccount.ServiceAccountId(),
c.wifConfig.Gcp().ProjectId(),
),
)
if err != nil {
return errors.Wrapf(err, "Failed to determine access policy of service account '%s'",
serviceAccount.ServiceAccountId())
}
if policy.HasRole(
gcp.PolicyMember(fmt.Sprintf("serviceAccount:%s", c.wifConfig.Gcp().ImpersonatorEmail())),
impersonatorRole,
) {
return nil
}

policy.AddRole(
gcp.PolicyMember(fmt.Sprintf("serviceAccount:%s", c.wifConfig.Gcp().ImpersonatorEmail())),
impersonatorRole,
)
if err := c.gcpClient.SetServiceAccountAccessPolicy(
ctx,
policy,
); err != nil {
return errors.Wrapf(err, "Failed to attach impersonator to service account '%s'",
serviceAccount.ServiceAccountId())
}

log.Printf("Impersonation access granted to service account '%s'",
serviceAccount.ServiceAccountId())
return nil
}

func (c *shim) attachWorkloadIdentityPool(
ctx context.Context,
serviceAccount *cmv1.WifServiceAccount,
) error {
policy, err := c.gcpClient.GetServiceAccountAccessPolicy(
ctx,
gcp.FmtSaResourceId(
serviceAccount.ServiceAccountId(),
c.wifConfig.Gcp().ProjectId(),
),
)
if err != nil {
return errors.Wrapf(err, "Failed to determine access policy of service account '%s'",
serviceAccount.ServiceAccountId())
}

var modified bool
openshiftNamespace := serviceAccount.CredentialRequest().SecretRef().Namespace()
for _, openshiftServiceAccount := range serviceAccount.CredentialRequest().ServiceAccountNames() {
principal := fmt.Sprintf(
"principal://iam.googleapis.com/projects/%s/"+
"locations/global/workloadIdentityPools/%s/"+
"subject/system:serviceaccount:%s:%s",
c.wifConfig.Gcp().ProjectNumber(),
c.wifConfig.Gcp().WorkloadIdentityPool().PoolId(),
openshiftNamespace, openshiftServiceAccount,
)

if !policy.HasRole(
gcp.PolicyMember(principal),
gcp.RoleName(workloadIdentityUserRole),
) {
modified = true

policy.AddRole(
gcp.PolicyMember(principal),
gcp.RoleName(workloadIdentityUserRole),
)
}
}

if modified {
if err := c.gcpClient.SetServiceAccountAccessPolicy(
ctx,
policy,
); err != nil {
return errors.Wrapf(err, "Failed to attach federated access on service account '%s'",
serviceAccount.ServiceAccountId())
}
log.Printf("Federated access granted to service account '%s'",
serviceAccount.ServiceAccountId())
}
return nil
}
1 change: 1 addition & 0 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ require (
cloud.google.com/go/compute/metadata v0.3.0 // indirect
github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect
github.com/99designs/keyring v1.2.2 // indirect
github.com/MicahParks/jwkset v0.5.20 // indirect
github.com/alessio/shellescape v1.4.1 // indirect
github.com/aws/aws-sdk-go v1.44.110 // indirect
github.com/aymerick/douceur v0.2.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03
github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo=
github.com/Masterminds/semver/v3 v3.1.1 h1:hLg3sBzpNErnxhQtUy/mmLR2I9foDujNK030IGemrRc=
github.com/Masterminds/semver/v3 v3.1.1/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs=
github.com/MicahParks/jwkset v0.5.20 h1:gTIKx9AofTqQJ0srd8AL7ty9NeadP5WUXSPOZadTpOI=
github.com/MicahParks/jwkset v0.5.20/go.mod h1:q8ptTGn/Z9c4MwbcfeCDssADeVQb3Pk7PnVxrvi+2QY=
github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2 h1:+vx7roKuyA63nhn5WAunQHLTznkw5W8b1Xc0dNjp83s=
github.com/Netflix/go-expect v0.0.0-20220104043353-73e0943537d2/go.mod h1:HBCaDeC1lPdgDeDbhX8XFpy1jqjK0IBG8W5K+xYqA0w=
github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc=
Expand Down
Loading

0 comments on commit 4e0c787

Please sign in to comment.