Adds Roles for IC resource access requests

[tcsc]

No description provided.

[espadolini]

Are we just going to keep adding preset roles? Is there a documented list of roles that people should expect to see in their cluster? We've had customers that were rightfully weirded out by the addition of roles that they didn't add.

[tcsc]

Are we just going to keep adding preset roles?

@espadolini - A good question, to which I don't have a good answer. I've basically followed the pattern used in other integrations, but I'm open to alternative suggestions on how to grant/deny access to IC resources with access requests.

I did consider just adding the appropriate conditions to requester and access, but I'm not sure that would be any better.

[smallinsky]

Are we just going to keep adding preset roles?

@espadolini - A good question, to which I don't have a good answer. I've basically followed the pattern used in other integrations, but I'm open to alternative suggestions on how to grant/deny access to IC resources with access requests.

I did consider just adding the appropriate conditions to requester and access, but I'm not sure that would be any better.

I agree that the creation of additional roles by Teleport under the hood might be unexpected behavior for Teleport users , we can update our documentation and AWS Identity Center enrollment flow to clearly explain that the integration will create these roles to support the access request flow that we make clear that after enrolling AWS IC those roles are expected and will be created internally by teleport

[smallinsky]

Doe the RO rule for `KindIdentityCenter is really needs to access AWS IC resource ?

[tcsc]

Things stop working when I take it out, so... :-)

Looking at the implementation of ListResources(), the role access rules are explicitly checked, so I think it's necessary.

[smallinsky]

wonder why we need the separate AWS IC requester role for that

After taking a look at our code we can just extend the NewPresetRequesterRole that currently grands access request role for PresetGroupAccessRoleName and Okta:

teleport/lib/services/presets.go

Line 706 in 94dc1a9

teleport.SystemOktaAccessRoleName,

Does reusing NewPresetReviewerRole and NewPresetRequesterRole this will work in AWS IC case ?

[r0mant]

I agree with other commenters - can we please update the existing preset roles to allow requesting access to IC resources rather than adding another set of presets?

It's more than just about having fewer preset roles, new presets will also have ripple effects on the documentation guides, etc. If we update existing ones, things will work out of the box, docs won't need as much updating, etc.

[r0mant]

Hrm, wouldn't this assign people with this role to each account and permission set pair? Don't we want to only grant access request privileges to them?

[smallinsky]

Yes, this will ensure that anyone with the appropriate access role is assigned to WS IC resources.

However we should consider following the existing pattern:

// defaultAllowAccessRequestConditions has the access request conditions that should be set as default when they were
// not explicitly defined.
func defaultAllowAccessRequestConditions(enterprise bool) map[string]*types.AccessRequestConditions {
	if enterprise {
		return map[string]*types.AccessRequestConditions{
			teleport.PresetRequesterRoleName: {
				SearchAsRoles: []string{
					teleport.PresetAccessRoleName,
					teleport.PresetGroupAccessRoleName,
				},
			},
			teleport.SystemOktaRequesterRoleName: {
				SearchAsRoles: []string{
					teleport.SystemOktaAccessRoleName,
				},
				MaxDuration: types.NewDuration(MaxAccessDuration),
			},
		}
	}

	return map[string]*types.AccessRequestConditions{}
}

// defaultAllowAccessReviewConditions has the access review conditions that should be set as default when they were
// not explicitly defined.
func defaultAllowAccessReviewConditions(enterprise bool) map[string]*types.AccessReviewConditions {
	if enterprise {
		return map[string]*types.AccessReviewConditions{
			teleport.PresetReviewerRoleName: {
				PreviewAsRoles: []string{
					teleport.PresetAccessRoleName,
					teleport.PresetGroupAccessRoleName,
				},
				Roles: []string{
					teleport.PresetAccessRoleName,
					teleport.PresetGroupAccessRoleName,
				},
			},
		}
	}

	return map[string]*types.AccessReviewConditions{}
}

To extend this we just need to add PresetAWSIdentityCenterAccessRoleName in both defaultAllowAccessRequestConditions and defaultAllowAccessReviewConditions. This ensures that the access role does not interfere with AWS IC resources.

[r0mant]

Yeah I agree let's do that as it's safer.

@tcsc Sorry for going back and forth but I think it makes sense to have preset role for AWS IC resources to make sure users don't accidentally grant everyone access to everything by assigning access role.

[tcsc]

I have to admit that I was a bit concerned about that myself.

[r0mant]

This is very confusing function signature, can we have it return whether the role changed or not (boolean argument) rather than modify the passed in pointer?

[r0mant]

with a comment

[r0mant]

Hmm isn't this the equivalent (and cleaner) or am I missing something?

Same below.

Suggested change

	target.Roles, rolesUpdated = mergeStrings(target.Roles, defaults.Roles)
	changed = changed || rolesUpdated
	target.SearchAsRoles, rolesUpdated = mergeStrings(target.SearchAsRoles, defaults.SearchAsRoles)
	changed = changed || rolesUpdated
	target.Roles, changed = mergeStrings(target.Roles, defaults.Roles)
	target.SearchAsRoles, changed = mergeStrings(target.SearchAsRoles, defaults.SearchAsRoles)

[tcsc]

If the first mergeStrings call changes the roles but the second one doesn't, then the changed flag will be incorrectly reset and the change won't be propagated.

[r0mant]

Suggested change

	target.Roles, rolesUpdated = mergeStrings(target.Roles, defaults.Roles)
	changed = changed || rolesUpdated
	target.PreviewAsRoles, rolesUpdated = mergeStrings(target.PreviewAsRoles, defaults.PreviewAsRoles)
	changed = changed || rolesUpdated
	target.Roles, changed = mergeStrings(target.Roles, defaults.Roles)
	target.PreviewAsRoles, changed = mergeStrings(target.PreviewAsRoles, defaults.PreviewAsRoles)

[public-teleport-github-review-bot]

@tcsc See the table below for backport results.

Branch	Result
branch/v17	Failed