From d4c34098030a7e1fe2f96c90dacd9d09b5925b81 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 13 Apr 2023 13:40:14 +0200 Subject: [PATCH] proxy: Rework oidc role mapper to allow multiple matching roles If multiple claims values have a valid matching for ocis roles, we'll pick the ocis role that appears first in the mapping configuration. --- .../unreleased/role-assignment-from-oidc.md | 23 ++++ services/proxy/README.md | 37 ++++-- services/proxy/pkg/command/server.go | 2 +- services/proxy/pkg/config/config.go | 10 +- .../pkg/config/defaults/defaultconfig.go | 10 +- services/proxy/pkg/userroles/oidcroles.go | 107 +++++++++--------- services/proxy/pkg/userroles/userroles.go | 5 +- 7 files changed, 121 insertions(+), 73 deletions(-) create mode 100644 changelog/unreleased/role-assignment-from-oidc.md diff --git a/changelog/unreleased/role-assignment-from-oidc.md b/changelog/unreleased/role-assignment-from-oidc.md new file mode 100644 index 00000000000..15c3b19b4e2 --- /dev/null +++ b/changelog/unreleased/role-assignment-from-oidc.md @@ -0,0 +1,23 @@ +Enhancement: Added possiblity to assign roles based on OIDC claims + +oCIS can now be configured to update a user's role assignment from the values of a claim provided +via the IDPs userinfo endpoint. The claim name and the mapping between claim values and ocis role +name can be configured via the configuration of the proxy service. Example: + +```yaml +role_assignment: + driver: oidc + oidc_role_mapper: + role_claim: ocisRoles + role_mapping: + - role_name: admin + claim_value: myAdminRole + - role_name: spaceadmin + claim_value: mySpaceAdminRole + - role_name: user + claim_value: myUserRole + - role_name: guest: + claim_value: myGuestRole +``` + +https://github.com/owncloud/ocis/pull/6048 diff --git a/services/proxy/README.md b/services/proxy/README.md index eae7359dd79..110d9266b55 100644 --- a/services/proxy/README.md +++ b/services/proxy/README.md @@ -50,10 +50,14 @@ role_assignment: oidc_role_mapper: role_claim: ocisRoles role_mapping: - admin: myAdminRole - user: myUserRole - spaceadmin: mySpaceAdminRole - guest: myGuestRole + - role_name: admin + claim_value: myAdminRole + - role_name: spaceadmin + claim_value: mySpaceAdminRole + - role_name: user + claim_value: myUserRole + - role_name: guest: + claim_value: myGuestRole ``` This would assign the role `admin` to users with the value `myAdminRole` in the claim `ocisRoles`. @@ -62,16 +66,27 @@ The role `user` to users with the values `myUserRole` in the claims `ocisRoles` Claim values that are not mapped to a specific ownCloud Infinite Scale role will be ignored. Note: An ownCloud Infinite Scale user can only have a single role assigned. If the configured -`role_mapping` and a user's claim values result in multiple possible roles for a user, an error -will be logged and the user will not be able to login. +`role_mapping` and a user's claim values result in multiple possible roles for a user, the order in +which the role mappings are defined in the configuration is important. The first role in the +`role_mappings` where the `claim_value` matches a value from the user's roles claim will be assigned +to the user. So if e.g. a user's `ocisRoles` claim has the values `myUserRole` and +`mySpaceAdminRole` that user will get the ocis role `spaceadmin` assigned (because `spaceadmin` +appears before `user` in the above sample configuration). -The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The `role_mapping` is: +If a user's claim values don't match any of the configured role mappings an error will be logged and +the user will not be able to login. + +The default `role_claim` (or `PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM`) is `roles`. The default `role_mapping` is: ```yaml -admin: ocisAdmin -user: ocisUser -spaceadmin: ocisSpaceAdmin -guest: ocisGuest +- role_name: admin + claim_value: ocisAdmin +- role_name: spaceadmin + claim_value: ocisSpaceAdmin +- role_name: user + claim_value: ocisUser +- role_name: guest: + claim_value: ocisGuest ``` ## Recommendations for Production Deployments diff --git a/services/proxy/pkg/command/server.go b/services/proxy/pkg/command/server.go index f51436121bc..d2aee9d878f 100644 --- a/services/proxy/pkg/command/server.go +++ b/services/proxy/pkg/command/server.go @@ -178,7 +178,7 @@ func loadMiddlewares(ctx context.Context, logger log.Logger, cfg *config.Config) userroles.WithRoleService(rolesClient), userroles.WithLogger(logger), userroles.WithRolesClaim(cfg.RoleAssignment.OIDCRoleMapper.RoleClaim), - userroles.WithRoleMapping(cfg.RoleAssignment.OIDCRoleMapper.RoleMapping), + userroles.WithRoleMapping(cfg.RoleAssignment.OIDCRoleMapper.RolesMap), userroles.WithAutoProvisonCreator(autoProvsionCreator), ) default: diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index 4c72ef4c5ed..a2d6daed6fc 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -135,8 +135,14 @@ type RoleAssignment struct { // OIDCRoleMapper contains the configuration for the "oidc" role assignment driber type OIDCRoleMapper struct { - RoleClaim string `yaml:"role_claim" env:"PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM" desc:"The OIDC claim used to create the users role assignment."` - RoleMapping map[string]string `yaml:"role_mapping" desc:"A mapping of ocis role names to PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM claim values. This setting can only be configured in the configuration file and not via environment variables."` + RoleClaim string `yaml:"role_claim" env:"PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM" desc:"The OIDC claim used to create the users role assignment."` + RolesMap []RoleMapping `yaml:"role_mapping" desc:"A list of mappings of ocis role names to PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM claim values. This setting can only be configured in the configuration file and not via environment variables."` +} + +// RoleMapping defines which ocis role matches a specific claim value +type RoleMapping struct { + RoleName string `yaml:"role_name" desc:"The name of an ocis role that this mapping should apply for."` + ClaimValue string `yaml:"claim_value" desc:"The value of the 'PROXY_ROLE_ASSIGNMENT_OIDC_CLAIM' that matches the role defined in 'role_name'."` } // PolicySelector is the toplevel-configuration for different selectors diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index 582dbd0da79..b3316083e15 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -60,11 +60,11 @@ func DefaultConfig() *config.Config { // this default is only relevant when Driver is set to "oidc" OIDCRoleMapper: config.OIDCRoleMapper{ RoleClaim: "roles", - RoleMapping: map[string]string{ - "admin": "ocisAdmin", - "spaceadmin": "ocisSpaceAdmin", - "user": "ocisUser", - "guest": "ocisGuest", + RolesMap: []config.RoleMapping{ + config.RoleMapping{RoleName: "admin", ClaimValue: "ocisAdmin"}, + config.RoleMapping{RoleName: "spaceadmin", ClaimValue: "ocisSpaceAdmin"}, + config.RoleMapping{RoleName: "user", ClaimValue: "ocisUser"}, + config.RoleMapping{RoleName: "guest", ClaimValue: "ocisGuest"}, }, }, }, diff --git a/services/proxy/pkg/userroles/oidcroles.go b/services/proxy/pkg/userroles/oidcroles.go index 0aee52d873b..647b216cc7d 100644 --- a/services/proxy/pkg/userroles/oidcroles.go +++ b/services/proxy/pkg/userroles/oidcroles.go @@ -34,47 +34,55 @@ func NewOIDCRoleAssigner(opts ...Option) UserRoleAssigner { // already has a different role assigned. func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *cs3.User, claims map[string]interface{}) (*cs3.User, error) { logger := ra.logger.SubloggerWithRequestID(ctx).With().Str("userid", user.GetId().GetOpaqueId()).Logger() - claimValueToRoleID, err := ra.oidcClaimvaluesToRoleIDs() + roleNamesToRoleIDs, err := ra.roleNamesToRoleIDs() if err != nil { - logger.Error().Err(err).Msg("Error mapping claims to roles ids") + logger.Error().Err(err).Msg("Error mapping role names to role ids") return nil, err } - roleIDsFromClaim := make([]string, 0, 1) - claimRoles, ok := claims[ra.rolesClaim].([]interface{}) + claimRolesRaw, ok := claims[ra.rolesClaim].([]interface{}) if !ok { - logger.Error().Err(err).Str("rolesClaim", ra.rolesClaim).Msg("No roles in user claims") - return nil, err + logger.Error().Str("rolesClaim", ra.rolesClaim).Msg("No roles in user claims") + return nil, errors.New("no roles in user claims") } + logger.Debug().Str("rolesClaim", ra.rolesClaim).Interface("rolesInClaim", claims[ra.rolesClaim]).Msg("got roles in claim") - for _, cri := range claimRoles { + claimRoles := map[string]struct{}{} + for _, cri := range claimRolesRaw { cr, ok := cri.(string) if !ok { err := errors.New("invalid role in claims") logger.Error().Err(err).Interface("claimValue", cri).Msg("Is not a valid string.") return nil, err } - id, ok := claimValueToRoleID[cr] - if !ok { - logger.Debug().Str("role", cr).Msg("No mapping for claim role. Skipped.") - continue - } - roleIDsFromClaim = append(roleIDsFromClaim, id) + + claimRoles[cr] = struct{}{} } - logger.Debug().Interface("roleIDs", roleIDsFromClaim).Msg("Mapped claim roles to roleids") - switch len(roleIDsFromClaim) { - default: - err := errors.New("too many roles found in claims") - logger.Error().Err(err).Msg("Only one role per user is allowed.") + if len(claimRoles) == 0 { + err := errors.New("no roles set in claim") + logger.Error().Err(err).Msg("") return nil, err - case 0: - err := errors.New("no role in claim, maps to a ocis role") + } + + // the roleMapping config is supposed to have the role mappings ordered from the highest privileged role + // down to the lowest privileged role. Since ocis currently only can handle a single role assignment we + // pick the highest privileged role that matches a value from the claims + roleIDFromClaim := "" + for _, mapping := range ra.Options.roleMapping { + if _, ok := claimRoles[mapping.ClaimValue]; ok { + logger.Debug().Str("ocisRole", mapping.RoleName).Str("role id", roleNamesToRoleIDs[mapping.RoleName]).Msg("first matching role") + roleIDFromClaim = roleNamesToRoleIDs[mapping.RoleName] + break + } + } + + if roleIDFromClaim == "" { + err := errors.New("no role in claim maps to an ocis role") logger.Error().Err(err).Msg("") return nil, err - case 1: - // exactly one mapping. This is right } + assignedRoles, err := loadRolesIDs(ctx, user.GetId().GetOpaqueId(), ra.roleService) if err != nil { logger.Error().Err(err).Msg("Could not load roles") @@ -86,8 +94,9 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c return nil, err } logger.Debug().Interface("assignedRoleIds", assignedRoles).Msg("Currently assigned roles") - if len(assignedRoles) == 0 || (assignedRoles[0] != roleIDsFromClaim[0]) { - logger.Debug().Interface("assignedRoleIds", assignedRoles).Interface("newRoleIds", roleIDsFromClaim).Msg("Updating role assignment for user") + + if len(assignedRoles) == 0 || (assignedRoles[0] != roleIDFromClaim) { + logger.Debug().Interface("assignedRoleIds", assignedRoles).Interface("newRoleId", roleIDFromClaim).Msg("Updating role assignment for user") newctx, err := ra.prepareAdminContext() if err != nil { logger.Error().Err(err).Msg("Error creating admin context") @@ -95,14 +104,14 @@ func (ra oidcRoleAssigner) UpdateUserRoleAssignment(ctx context.Context, user *c } if _, err = ra.roleService.AssignRoleToUser(newctx, &settingssvc.AssignRoleToUserRequest{ AccountUuid: user.GetId().GetOpaqueId(), - RoleId: roleIDsFromClaim[0], + RoleId: roleIDFromClaim, }); err != nil { logger.Error().Err(err).Msg("Role assignment failed") return nil, err } } - user.Opaque = utils.AppendJSONToOpaque(user.Opaque, "roles", roleIDsFromClaim) + user.Opaque = utils.AppendJSONToOpaque(user.Opaque, "roles", []string{roleIDFromClaim}) return user, nil } @@ -136,32 +145,32 @@ func (ra oidcRoleAssigner) prepareAdminContext() (context.Context, error) { return newctx, nil } -type roleClaimToIDCache struct { - roleClaimToID map[string]string - lastRead time.Time - lock sync.RWMutex +type roleNameToIDCache struct { + roleNameToID map[string]string + lastRead time.Time + lock sync.RWMutex } -var roleClaimToID roleClaimToIDCache +var roleNameToID roleNameToIDCache -func (ra oidcRoleAssigner) oidcClaimvaluesToRoleIDs() (map[string]string, error) { +func (ra oidcRoleAssigner) roleNamesToRoleIDs() (map[string]string, error) { cacheTTL := 5 * time.Minute - roleClaimToID.lock.RLock() + roleNameToID.lock.RLock() - if !roleClaimToID.lastRead.IsZero() && time.Since(roleClaimToID.lastRead) < cacheTTL { - defer roleClaimToID.lock.RUnlock() - return roleClaimToID.roleClaimToID, nil + if !roleNameToID.lastRead.IsZero() && time.Since(roleNameToID.lastRead) < cacheTTL { + defer roleNameToID.lock.RUnlock() + return roleNameToID.roleNameToID, nil } ra.logger.Debug().Msg("refreshing roles ids") // cache needs Refresh get a write lock - roleClaimToID.lock.RUnlock() - roleClaimToID.lock.Lock() - defer roleClaimToID.lock.Unlock() + roleNameToID.lock.RUnlock() + roleNameToID.lock.Lock() + defer roleNameToID.lock.Unlock() // check again, another goroutine might have updated while we "upgraded" the lock - if !roleClaimToID.lastRead.IsZero() && time.Since(roleClaimToID.lastRead) < cacheTTL { - return roleClaimToID.roleClaimToID, nil + if !roleNameToID.lastRead.IsZero() && time.Since(roleNameToID.lastRead) < cacheTTL { + return roleNameToID.roleNameToID, nil } // Get all roles to find the role IDs. @@ -183,16 +192,10 @@ func (ra oidcRoleAssigner) oidcClaimvaluesToRoleIDs() (map[string]string, error) newIDs := map[string]string{} for _, role := range res.Bundles { ra.logger.Debug().Str("role", role.Name).Str("id", role.Id).Msg("Got Role") - roleClaim, ok := ra.roleMapping[role.Name] - if !ok { - err := errors.New("Incomplete role mapping") - ra.logger.Error().Err(err).Str("role", role.Name).Msg("Role not mapped to a claim value") - return map[string]string{}, err - } - newIDs[roleClaim] = role.Id + newIDs[role.Name] = role.Id } - ra.logger.Debug().Interface("roleMap", newIDs).Msg("Claim Role to role ID map") - roleClaimToID.roleClaimToID = newIDs - roleClaimToID.lastRead = time.Now() - return roleClaimToID.roleClaimToID, nil + ra.logger.Debug().Interface("roleMap", newIDs).Msg("Role Name to role ID map") + roleNameToID.roleNameToID = newIDs + roleNameToID.lastRead = time.Now() + return roleNameToID.roleNameToID, nil } diff --git a/services/proxy/pkg/userroles/userroles.go b/services/proxy/pkg/userroles/userroles.go index 8c2dd431fe1..55d8afc7e13 100644 --- a/services/proxy/pkg/userroles/userroles.go +++ b/services/proxy/pkg/userroles/userroles.go @@ -7,6 +7,7 @@ import ( "github.com/owncloud/ocis/v2/ocis-pkg/log" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" "github.com/owncloud/ocis/v2/services/proxy/pkg/autoprovision" + "github.com/owncloud/ocis/v2/services/proxy/pkg/config" ) //go:generate mockery --name=UserRoleAssigner @@ -26,7 +27,7 @@ type UserRoleAssigner interface { type Options struct { roleService settingssvc.RoleService rolesClaim string - roleMapping map[string]string + roleMapping []config.RoleMapping autoProvsionCreator autoprovision.Creator logger log.Logger } @@ -56,7 +57,7 @@ func WithRolesClaim(claim string) Option { } // WithRoleMapping configures the map of ocis role names to claims values -func WithRoleMapping(roleMap map[string]string) Option { +func WithRoleMapping(roleMap []config.RoleMapping) Option { return func(o *Options) { o.roleMapping = roleMap }